Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use is_recording flag in flask, django, tornado, boto, botocore instrumentations #1164

Merged
merged 10 commits into from
Sep 30, 2020

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Sep 25, 2020

Part of #1057

@lzchen lzchen requested a review from a team September 25, 2020 15:48
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, it looks pretty good. Please add the changelogs and I've added some comments in the review.

@@ -91,6 +91,8 @@ def collect_request_attributes(scope):

def set_status_code(span, status_code):
"""Adds HTTP response attributes to span using the status_code argument."""
if not span.is_recording():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's worth adding a decorator as a convenience

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly? I think the repetitive-ness of this task just seems obvious because we just didn't do it in the beginning. However, new instrumentations should be respecting this anyways, so I don't think it's that much overhead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking a decorator may help folks building future instrumentations group all the actions that should not occur when a span is not recording more cleanly 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice. There are some instrumentations that have some interim steps that always must be regardless of is_recording (which occur between some set_attr... methods). A decorator would be difficult to use in those cases.

@lzchen
Copy link
Contributor Author

lzchen commented Sep 29, 2020

Thanks for the change, it looks pretty good. Please add the changelogs and I've added some comments in the review.

I don't believe these needs CHANGELOG entries because they it's not really affecting the user.

@codeboten
Copy link
Contributor

I don't believe these needs CHANGELOG entries because they it's not really affecting the user.

I was thinking the change in behaviour might be unexpected (if anyone was setting is_recording to false and still seeing attributes), though I guess it would have been surprising for this to behave this way in the first place.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, I can go either ways on the update to the change logs, and I'm not blocking on the decorator.

Comment on lines +113 to +117
attributes = collect_request_attributes(environ)
for attr in self._traced_request_attrs:
value = getattr(request, attr, None)
if value is not None:
attributes[attr] = str(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@owais created a nifty utils function that you can use like this

Suggested change
attributes = collect_request_attributes(environ)
for attr in self._traced_request_attrs:
value = getattr(request, attr, None)
if value is not None:
attributes[attr] = str(value)
attributes = collect_request_attributes(environ)
attributes = extract_attributes_from_object(
request, self._traced_request_attrs, attributes
)

It's still in PR though so no need to use it if this is ready first.

@lzchen lzchen merged commit 5eb0598 into open-telemetry:master Sep 30, 2020
@lzchen lzchen deleted the flask branch September 30, 2020 14:37
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants